Automated Test: enhance-passkey-authentication-flow #311
+407
−24
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
closes #41242 closes #41008 Signed-off-by: mposolda <mposolda@gmail.com>
This change modifies the method signature to require a UserModel parameter for proper user context validation during conditional passkey checks.
📝 WalkthroughWalkthroughThis pull request refactors Keycloak's authentication flow to improve reauthentication handling for passkey scenarios. Changes include exposing a constant, consolidating conditional passkey checks into helper methods, introducing reauthentication setup utilities, and adding new test coverage for passkey-based reauthentication flows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AuthForm as Username/Password Form
participant Authenticator as AuthenticationFlowContext
participant Utils as AuthenticatorUtils
participant FormProvider as LoginFormsProvider
User->>AuthForm: Submit invalid password
AuthForm->>Authenticator: badPasswordHandler triggered
Authenticator->>Utils: setupReauthenticationInUsernamePasswordFormError(context)
Utils->>Authenticator: Read auth note (USER_SET_BEFORE_USERNAME_PASSWORD_AUTH)
Authenticator->>FormProvider: Hide username field
Authenticator->>FormProvider: Disable registration
FormProvider->>User: Display reauthentication error form
User->>AuthForm: Re-authenticate with credentials
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernameForm.java`:
- Line 47: The call to isConditionalPasskeysEnabled() in UsernameForm is missing
the required UserModel parameter and causes a compilation error; update the
conditional to call isConditionalPasskeysEnabled(context.getUser()) (you can
rely on the existing null check context.getUser() != null) so it matches the
inherited method signature isConditionalPasskeysEnabled(UserModel) from
UsernamePasswordForm and compiles correctly.
In
`@testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysUsernamePasswordFormTest.java`:
- Around line 277-333: The test reauthenticationOfUserWithoutPasskey currently
uses driver.findElement(...) inside broad try-catch blocks which swallows all
exceptions; replace those blocks by checking
driver.findElements(By.xpath("//form[`@id`='webauth']")).isEmpty() and asserting
true (or using an assertion like assertTrue/isEmpty) to verify the webauthn form
is absent instead of catching Exception. Update both occurrences in
reauthenticationOfUserWithoutPasskey (the two try-catch sections around checking
"//form[`@id`='webauth']") and keep existing assertions/flow
(loginPage.assertCurrent(), fail() removal) so test logic remains the same.
| @@ -44,7 +44,7 @@ public UsernameForm(KeycloakSession session) { | |||
|
|
|||
| @Override | |||
| public void authenticate(AuthenticationFlowContext context) { | |||
| if (context.getUser() != null) { | |||
| if (context.getUser() != null && !isConditionalPasskeysEnabled()) { | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for all isConditionalPasskeysEnabled method definitions
rg "isConditionalPasskeysEnabled" -B 2 -A 3 --type javaRepository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 3546
🏁 Script executed:
# Locate UsernameForm.java and its parent class
find . -name "UsernameForm.java" -o -name "UsernamePasswordForm.java" | head -20Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 282
🏁 Script executed:
# Check the class definition and parent class of UsernameForm
head -50 services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernameForm.javaRepository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 2047
🏁 Script executed:
# Check all methods in UsernamePasswordForm to confirm no overloads
grep -n "isConditionalPasskeysEnabled" services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernamePasswordForm.javaRepository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 293
Method signature mismatch — compilation error.
The call isConditionalPasskeysEnabled() is missing the required UserModel argument. The inherited method isConditionalPasskeysEnabled(UserModel user) in UsernamePasswordForm requires a parameter.
Proposed fix
- if (context.getUser() != null && !isConditionalPasskeysEnabled()) {
+ if (context.getUser() != null && !isConditionalPasskeysEnabled(context.getUser())) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (context.getUser() != null && !isConditionalPasskeysEnabled()) { | |
| if (context.getUser() != null && !isConditionalPasskeysEnabled(context.getUser())) { |
🤖 Prompt for AI Agents
In
`@services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernameForm.java`
at line 47, The call to isConditionalPasskeysEnabled() in UsernameForm is
missing the required UserModel parameter and causes a compilation error; update
the conditional to call isConditionalPasskeysEnabled(context.getUser()) (you can
rely on the existing null check context.getUser() != null) so it matches the
inherited method signature isConditionalPasskeysEnabled(UserModel) from
UsernamePasswordForm and compiles correctly.
Comment on lines
+277
to
+333
| // Test user re-authentication with password when passkeys feature enabled, but passkeys is not enabled for the realm. Passkeys should not be shown during re-authentication | ||
| @Test | ||
| public void reauthenticationOfUserWithoutPasskey() throws Exception { | ||
| // set passwordless policy for discoverable keys | ||
| try (Closeable c = getWebAuthnRealmUpdater() | ||
| .setWebAuthnPolicyRpEntityName("localhost") | ||
| .setWebAuthnPolicyRequireResidentKey(Constants.WEBAUTHN_POLICY_OPTION_YES) | ||
| .setWebAuthnPolicyUserVerificationRequirement(Constants.WEBAUTHN_POLICY_OPTION_REQUIRED) | ||
| .setWebAuthnPolicyPasskeysEnabled(Boolean.TRUE) | ||
| .setWebAuthnPolicyPasskeysEnabled(Boolean.FALSE) | ||
| .update()) { | ||
|
|
||
| // Login with password | ||
| oauth.openLoginForm(); | ||
| WaitUtils.waitForPageToLoad(); | ||
|
|
||
| // WebAuthn elements not available | ||
| loginPage.assertCurrent(); | ||
| try { | ||
| MatcherAssert.assertThat(driver.findElement(By.xpath("//form[@id='webauth']")), nullValue()); | ||
| fail("Not expected to have webauthn button"); | ||
| } catch (Exception nsee) { | ||
| // expected | ||
| } | ||
|
|
||
| // Login with password | ||
| loginPage.login("test-user@localhost", getPassword("test-user@localhost")); | ||
| appPage.assertCurrent(); | ||
|
|
||
| events.clear(); | ||
|
|
||
| // Re-authentication now with prompt=login. Passkeys login should not be available on the page as this user does not have passkey | ||
| oauth.loginForm() | ||
| .prompt(OIDCLoginProtocol.PROMPT_VALUE_LOGIN) | ||
| .open(); | ||
| WaitUtils.waitForPageToLoad(); | ||
|
|
||
| loginPage.assertCurrent(); | ||
| assertEquals("Please re-authenticate to continue", loginPage.getInfoMessage()); | ||
| try { | ||
| MatcherAssert.assertThat(driver.findElement(By.xpath("//form[@id='webauth']")), nullValue()); | ||
| fail("Not expected to have webauthn button"); | ||
| } catch (Exception nsee) { | ||
| // expected | ||
| } | ||
|
|
||
| // Login with password | ||
| loginPage.login(getPassword("test-user@localhost")); | ||
| appPage.assertCurrent(); | ||
|
|
||
| UserRepresentation testUser = ApiUtil.findUserByUsernameId(testRealm(), "test-user@localhost").toRepresentation(); | ||
|
|
||
| events.expectLogin() | ||
| .user(testUser.getId()) | ||
| .detail(Details.USERNAME, testUser.getUsername()) | ||
| .detail(WebAuthnConstants.USER_VERIFICATION_CHECKED, nullValue()) | ||
| .assertEvent(); | ||
|
|
||
| logout(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "PasskeysUsernamePasswordFormTest.java" | head -5Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 229
🏁 Script executed:
cat -n testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysUsernamePasswordFormTest.java | sed -n '277,333p'Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 2901
🏁 Script executed:
# Search for similar patterns of findElement with try-catch in test files
rg "findElement.*nullValue" testsuite/ -A 3 -B 1 --max-count 10Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 2013
🏁 Script executed:
# Check WebDriver usage patterns with findElements in the codebase
rg "findElements\(" testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/ -A 2 --max-count 15Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 6857
Use findElements().isEmpty() instead of catching broad Exception with findElement().
The current try-catch blocks swallow all exceptions indiscriminately, which can mask real WebDriver failures. Catching Exception is overly broad; findElement() throws NoSuchElementException specifically when an element is not found. The safer approach is to use findElements(), which returns an empty list instead of throwing, and then assert emptiness. This pattern is already used elsewhere in the test suite and aligns with WebDriver best practices.
Suggested fix
Replace both occurrences (lines 291-296 and 312-317) with:
- try {
- MatcherAssert.assertThat(driver.findElement(By.xpath("//form[`@id`='webauth']")), nullValue());
- fail("Not expected to have webauthn button");
- } catch (Exception nsee) {
- // expected
- }
+ Assert.assertTrue("Not expected to have webauthn button",
+ driver.findElements(By.xpath("//form[`@id`='webauth']")).isEmpty());🤖 Prompt for AI Agents
In
`@testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysUsernamePasswordFormTest.java`
around lines 277 - 333, The test reauthenticationOfUserWithoutPasskey currently
uses driver.findElement(...) inside broad try-catch blocks which swallows all
exceptions; replace those blocks by checking
driver.findElements(By.xpath("//form[`@id`='webauth']")).isEmpty() and asserting
true (or using an assertion like assertTrue/isEmpty) to verify the webauthn form
is absent instead of catching Exception. Update both occurrences in
reauthenticationOfUserWithoutPasskey (the two try-catch sections around checking
"//form[`@id`='webauth']") and keep existing assertions/flow
(loginPage.assertCurrent(), fail() removal) so test logic remains the same.
Sign in
to join this conversation on GitHub.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
New Features
Tests